Skip to content

fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553

Open
aaronjmars wants to merge 1 commit into
obra:devfrom
aaronjmars:security/host-header-validation
Open

fix(brainstorm-server): validate Host header to defeat DNS rebinding#1553
aaronjmars wants to merge 1 commit into
obra:devfrom
aaronjmars:security/host-header-validation

Conversation

@aaronjmars
Copy link
Copy Markdown

@aaronjmars aaronjmars commented May 15, 2026

What problem are you trying to solve?

The brainstorm companion server (skills/brainstorming/scripts/server.cjs) accepts HTTP and WebSocket connections without validating the Host header. Combined with the listener's binding to 127.0.0.1, that's enough to keep the TCP socket "local" — but it does not stop a DNS-rebinding attack:

  1. User starts a brainstorm session; server binds to ws://127.0.0.1:<port> and the user opens the printed URL in their browser.
  2. In another tab the user lands on https://attacker.example, which has its own DNS server return its hostname pointing first to its own IP, then re-resolves it to 127.0.0.1 after the page loads (DNS rebind).
  3. JavaScript on attacker.example issues fetch('http://attacker.example:<port>/') and fetch('/files/...') — the TCP connection now lands on the loopback brainstorm server, but the browser sends Host: attacker.example and treats the response as same-origin.
  4. The server happily serves the active screen HTML and /files/* content. The attacker reads brainstorm screens and any file the agent dropped in CONTENT_DIR.
  5. Same rebind allows a WebSocket upgrade and a {type: "click", choice: "<attacker text>"} send, which the server appends to state_dir/events. On the next turn the agent reads that file as the user's selection.

Issue #1014 describes the WebSocket-side variant of this; PR #1110 covers the WebSocket Origin axis but leaves both HTTP info-disclosure paths and the Host-axis defense on WS in place. This PR closes the Host-header gap.

Reproduction loop (without the fix), using the existing test harness as the "attacker":

$ cd tests/brainstorm-server && npm test
# Old run: GET / with Host: evil.example:3334 → 200, body contains the
# brainstorm screen. WebSocket upgrade with Host: evil.example:3334 →
# 101 Switching Protocols, ws.send({choice:"x"}) lands in state/events.

What does this PR change?

Adds a single Host-header allowlist (buildAllowedHosts() + isHostAllowed()) in server.cjs and applies it before any work in handleRequest() (returns 421 Misdirected Request) and before the 101 handshake in handleUpgrade() (destroys the socket). Default allowlist: localhost, 127.0.0.1, [::1] (bare and with :PORT) plus the configured BRAINSTORM_HOST / BRAINSTORM_URL_HOST. Operators can extend via BRAINSTORM_ALLOWED_HOSTS (comma-separated) for tunneled / containerized setups that already exist in visual-companion.md.

Is this change appropriate for the core library?

Yes — skills/brainstorming/scripts/server.cjs is a first-party companion server shipped with core. DNS-rebinding is not a project-specific concern; every brainstorm session running anywhere is exposed without this gate. The fix introduces no new dependencies, no third-party integrations, and no configuration the user is forced to set (defaults work in every documented setup).

What alternatives did you consider?

  • Session-token URL (the approach in closed PR harden brainstorming session access and cleanup boundaries #685): much broader change — touches start-server.sh, visual-companion.md, helper.js, the URL the agent prints, and how the user opens the page. PR harden brainstorming session access and cleanup boundaries #685 was bundled with permissions/cleanup/lockfile concerns and got closed quickly. A token approach is also strictly defense-in-depth on top of the Host check — DNS rebinding is solved by checking Host, regardless of token presence.
  • Origin check on the WebSocket only (PR brainstorm-server: validate websocket origin before upgrade #1110): closes WS injection but leaves / and /files/* info-disclosure open. HTTP same-origin requests under DNS rebind don't send Origin, so an Origin check on the HTTP path would silently pass attacker traffic. Host validation is the canonical answer for the HTTP axis.
  • Drop /files/* entirely: removes one disclosure path but not the other (/ still serves the current screen). Also a much more invasive behavioral change.

The Host allowlist is the minimum surgical fix that closes the rebinding surface end-to-end on both HTTP and WS paths.

Does this PR contain multiple unrelated changes?

No. Single concern: validate the Host header against an allowlist before processing any HTTP request or completing a WebSocket upgrade. Two files touched — the server (+59) and the test file (+76) — both load-bearing for the same change.

Existing PRs

Environment tested

Harness (e.g. Claude Code, Cursor) Harness version Model Model version/ID
Claude Code (GitHub Actions runner via Aeon) 2.x Claude Opus claude-opus-4-7

New harness support (required if this PR adds a new harness)

N/A — no new harness. The existing using-superpowers bootstrap is untouched.

Evaluation

  • Initial prompt: an Aeon vuln-scanner skill run against trending repos selected obra/superpowers from today's GitHub trending list (No marketplace.json? #3, ★191,721). After semgrep / trufflehog / osv-scanner came back clean, manual review of the brainstorm companion server surfaced the missing Host gate.
  • Eval sessions after the change: 2 full runs of cd tests/brainstorm-server && npm install && npm test. Both: 31 passed, 0 failed.
  • Before vs after (same harness):
    • Before: GET / Host: evil.example:<port>200 + brainstorm screen body returned. GET /files/<any> Host: evil.example200/404 depending on file. ws://evil.example:<port> via DNS-rebind hop → upgrade succeeds, server accepts {choice: "..."} and writes it to state/events.
    • After: same three probes return 421 / 421 / socket destroy with no 101. Legitimate Host: localhost:<port>, 127.0.0.1:<port>, and bare-loopback Hosts continue to return 200 and complete the WS handshake.

Adversarial cases the new tests cover: foreign-Host on /, foreign-Host on /files/*, and a forged-Host WebSocket upgrade routed back to the loopback listener via a custom DNS lookup.

Rigor

  • If this is a skills change: I used superpowers:writing-skills and completed adversarial pressure testing (paste results below)
  • This change was tested adversarially, not just on the happy path
  • I did not modify carefully-tuned content (Red Flags table, rationalizations, "human partner" language) without extensive evals showing the change is an improvement

This is a server-code change, not a skills-prose change — the writing-skills checkbox is intentionally left unchecked. Adversarial coverage lives in the new tests (Host: evil.example, Host: attacker.example, ws://evil.example rebinding to 127.0.0.1).

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

Initially drafted by Aeon (an autonomous Claude Code agent); the complete 135-line diff has since been reviewed in full by a human (Aaron) before this update. Happy to address any feedback.


Filed by Aeon on behalf of @aaronjmars.

@aaronjmars
Copy link
Copy Markdown
Author

Friendly bump @obra — small Host-header validation fix to close the DNS-rebinding window on brainstorm-server. Happy to address any feedback when you have a moment.

@aaronjmars
Copy link
Copy Markdown
Author

Friendly second nudge @obra — small Host-header validation in brainstorm-server to close the DNS-rebinding window. Mergeable, no conflicts. Happy to iterate on feedback whenever you have a moment.

@obra obra added bug Something isn't working brainstorming Brainstorming skill and visual companion needs-rebase-to-dev-branch PR targets main but should target dev labels May 23, 2026
@YOMXXX
Copy link
Copy Markdown

YOMXXX commented May 24, 2026

Review note after reading the PR body and diff:

The security issue this addresses looks real, and Host allowlisting is the right axis for DNS rebinding on the HTTP / and /files/* paths. The implementation is also fairly contained: one allowlist helper in server.cjs, applied before HTTP handling and before WebSocket upgrade, plus focused tests for foreign Host rejection.

A couple of review blockers / cleanup items remain before this is easy to merge:

I did not find a correctness issue in the allowlist logic from the patch alone. The one thing I would re-run after rebasing to dev is the full brainstorm-server test file, because the WebSocket rejection test asserts the client observes an error; that should be verified on the current dependency/runtime after any rebase.

Allowlist the HTTP `Host` header on both the request path and the
WebSocket upgrade. Default allowlist covers `localhost`, `127.0.0.1`,
`[::1]` (with and without :PORT), plus the configured `BRAINSTORM_HOST`
and `BRAINSTORM_URL_HOST`. Operators can extend with
`BRAINSTORM_ALLOWED_HOSTS` (comma-separated) for tunneled setups.

Without this gate, a page on another origin can DNS-rebind its own
hostname to 127.0.0.1, hit `/` or `/files/*` on the running brainstorm
companion server, and read the active screen + content files even
though the listener is loopback-only. The same rebind would also let
the page complete a WebSocket upgrade and inject `{choice: ...}`
events into `state_dir/events`, which the agent reads as the user's
selection.

PR obra#1110 / issue obra#1014 already cover the WebSocket Origin axis. This
PR adds the complementary Host axis, which is the canonical defense
against DNS rebinding (Origin alone doesn't cover `/files/*`).

Tests: 6 new cases in tests/brainstorm-server/server.test.js cover
loopback accept, foreign-Host 421, /files/* foreign-Host 421, and a
forged-Host WebSocket upgrade. Existing 25 tests still pass (31/31).

Detected by: Aeon (https://github.com/aaronjmars/aeon-aaron) +
manual review against the brainstorm-server attack surface.
Severity: medium (cross-origin info disclosure + agent input
injection via DNS rebinding).
CWE-346 (Origin Validation Error), CWE-350 (Reliance on Reverse DNS
Resolution for a Security-Critical Action).
@aaronjmars aaronjmars changed the base branch from main to dev May 25, 2026 13:24
@aaronjmars aaronjmars force-pushed the security/host-header-validation branch from 9b59c01 to 5b018e0 Compare May 25, 2026 13:25
@aaronjmars
Copy link
Copy Markdown
Author

Thanks for the careful review @YOMXXX — addressed all three:

  • Retargeted + rebased onto dev. Base is now dev and the branch is rebased on top of it (single commit, clean — main is fully contained in dev so no conflicts).
  • Re-ran the full brainstorm-server suite on dev: 31/31 pass, including the foreign-Host WebSocket rejection test you flagged (rejects WebSocket upgrade from foreign Host — the client points at ws://evil.example:PORT with the TCP lookup forced back to 127.0.0.1, and the upgrade is correctly refused).
  • Human-review checkbox is now checked — the complete 135-line diff has been reviewed in full by a human, and I updated the note accordingly.

And agreed on the scope distinction you raised: this is HTTP+WS Host allowlisting (DNS rebinding), which is orthogonal to #1110's WebSocket Origin check — not a duplicate. Happy to iterate on anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

brainstorming Brainstorming skill and visual companion bug Something isn't working needs-rebase-to-dev-branch PR targets main but should target dev

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants